fix: don't close shared session store in runtime.Close#2879
Conversation
docker-agent
left a comment
docker-agent
left a comment
There was a problem hiding this comment.
Assessment: 🟢 APPROVE
The fix correctly addresses the shared session store lifecycle issue. The pattern for lazy initialization in is a sound approach, is properly deferred in , and the contract update is clearly documented. No resource leaks, double-close, or use-after-close issues were identified in the changed code.
docker-agent
left a comment
docker-agent
left a comment
There was a problem hiding this comment.
Assessment: 🟡 NEEDS ATTENTION
The fix correctly moves session-store ownership to the backend layer, resolving the use-after-close bug (#2872). One potential data race was found in the new localBackend.Close() method.
| } | ||
|
|
||
| func (b *localBackend) Close() error { | ||
| if b.store == nil { |
There was a problem hiding this comment.
[MEDIUM] Potential data race: Close() reads b.store outside sync.Once
localBackend.Close() reads b.store directly (line 130) without any synchronization, while sessionStore() writes b.store inside storeOnce.Do(...). The Go memory model only guarantees that the write to b.store inside the Do closure is visible after Do returns — it does not provide a happens-before guarantee for concurrent reads of b.store that happen outside of Do.
If Close() races with an in-progress storeOnce.Do() call (e.g., a session is still being initialized when shutdown begins), the Go race detector would flag this.
Suggested fix — either use storeOnce.Do to query the store in Close() as well, or protect b.store with a mutex:
func (b *localBackend) Close() error {
b.storeOnce.Do(func() {}) // ensure the write, if any, has completed
if b.store == nil {
return nil
}
return b.store.Close()
}Or, more robustly, add a sync.Mutex to guard access to b.store in both sessionStore and Close.
aheritier
added
area/sessions
3 participants
The TUI was failing to open
/sessionsafter switching to a session with a different working directory, with the error "failed to load sessions: sql: database is closed". This happened because when the TUI swapped in a new runtime for the different working directory, it canceled the old runtime's context and ran cleanup, which calledrt.Close()→r.sessionStore.Close(). However, the spawned new runtime was given the same SQLite store (viacreateSessionSpawnerreusingrt.SessionStore()), so closing it from the old runtime broke every subsequent read against that store.The fix moves session store ownership to the backend layer, which now owns its lifecycle independently. The
Runtime.Close()contract is updated to make explicit that the session store is owned by the embedder and may be shared.localBackendnow lazily opens the SQLite store on firstCreateSessioncall (viasync.Once) and closes it once in a newbackend.Close()method. The runtime no longer closes the store it was given, and the TUI defersb.Close()to clean up resources when the entire session ends.Fixes #2872